-
Notifications
You must be signed in to change notification settings - Fork 3
[feat] 퀴즈 조회, 수정, 삭제 API 구현 #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| @RequestParam(required = false) String creator) { | ||
|
|
||
| Pageable pageable = PageRequest.of(page - 1, size); | ||
| QuizListPageResponse quizzes = quizService.getQuizzes(title, creator, pageable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이부분은 지금 퀴즈리스트에 대한 정렬이 적용이 된건가요?.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
퀴즈 리스트에 대한 정렬은 적용되지 않았습니다 !
따로 정한 기준은 없었던 것 같은데,,, 제가 기억이 안 나는 걸까요..? 혹시 어떤 정렬을 해야 할까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[L4-변경제안]
최신순, 인기순 정렬을 변경하는 기능은 정의하지 않았지만, 디폴트 정렬은 있어야 사용자에게 일관적인 리스트를 보여줄 수 있다고 생각합니다!(명시하지않아서 디폴트 정렬로 나오는 것 같지만, 안정적으로 명시하는게 좋다는 의견입니다) 현재 1번부터 나오는 asc라면 desc로 정렬하면(최신순) 어떨까하는 의견입니다. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오..! 그렇군요. 페이징을 처음해봐서 정렬까지 같이 처리해주는 기능이 있는지 몰랐는데 디폴트로 정렬기준이 있다면 아주 유용하고 좋을 것 같습니다. 의견 반영해서 커밋 다시 올렸습니다. 좋은 의견 감사합니다 !!
jiwon1217
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다 ! 참고할만한 리뷰 남겼습니다
| try { | ||
| boolean deleted = deleteIfExists(filePath); | ||
| if (deleted) { | ||
| System.out.println("기존 썸네일 삭제 완료 : " + filePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[L4-변경제안]
출력문을 log로 대체하는 것이 좋아보입니다 !
[참고 링크] https://hudi.blog/do-not-use-system-out-println-for-logging/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
뭔가 @Slf4j 라는 추가적인 어노테이션을 달지 않아도 System.out.println로 로그를 찍어볼 수 있는 게 아닌가? 하고 생각했었는데,,, 생각해보니 제공되는 정보의 양이 차이가 많이나는군요 !!
좋은 인사이트 감사합니다. 수정하겠습니다 !
| Page<Quiz> quizzes; | ||
|
|
||
| // 검색어가 있을 때 | ||
| if (title != null && !title.isBlank()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[L4-변경제안]
string 조건 비교 시 Apache Commons에서 제공하는 유틸 메서드를 사용하면 조건문을 간단하게 표현할 수 있습니다 !
StringUtils.isBlack() 하나로 처리 가능합니다 !
[참고링크] https://hyeri0903.tistory.com/235
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
와우 저도 하나 얻어갑니다.. 💨
[L4-변경제안] string 조건 비교 시 Apache Commons에서 제공하는 유틸 메서드를 사용하면 조건문을 간단하게 표현할 수 있습니다 ! StringUtils.isBlack() 하나로 처리 가능합니다 ! [참고링크] https://hyeri0903.tistory.com/235
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
와 .. ! 저도 저 코드가 좀 안 예뻐서 마음에 안 들었는데 해결방법이 있었군요.. 굉장합니다.. 굉장해..
감사합니다 ! 이 부분도 수정하도록 하겠습니다.
sehee123
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다! 🧚♀️
| } | ||
|
|
||
| private void deleteOldThumbnailFileIfNeeded(String oldFilename) { | ||
| if (oldFilename.contains("default")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[L5-참고의견]
"default"를 상수로 만들어 사용하면 안전할 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확인했습니다 ! 이 부분도 수정하겠습니다 !
| } | ||
| } | ||
|
|
||
| private void deleteOldThumbnailFileIfNeeded(String oldFilename) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[L5-참고의견]
메서드 명을 deleteFile로 간단하게 정의해도 괜찮을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
옹 그렇군요 ! 흠 그래도 일반 파일을 지우는 메서드가 아니라 필요없는 썸네일 파일을 지우는 거라.. deleteOldThumbnailFile이나 deleteThumbnailFile로 가보겠습니다 !! 좋은 의견 감사합니다 :)
| Page<Quiz> quizzes; | ||
|
|
||
| // 검색어가 있을 때 | ||
| if (StringUtils.isBlank(title)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[L5-참고의견]
StringUtils의 사용은 Apache Commons를 주로 사용하는 것 같습니다. (멘토링 코드 리뷰 때 알게 됐습니다)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헉 import가 잘못됐네요 ! 감사합니다 ! 혹시 멘토링 때 그 차이점이 뭐라고 하셨나요?
🛰️ Issue Number
🪐 작업 내용
1. 퀴즈 삭제
2. 퀴즈 수정
null이 아닐 시, 변경되었다고 여깁니다.3. 퀴즈 조회
null이지 않고, 빈 문자열이 없을 때 검색어가 있다고 판단합니다.포스트맨에서 모두 응답이 잘 나오는 것을 확인하였습니다.
📚 Reference
✅ Check List